Add support for listing sources on customers #1064
Conversation
|
|
||
| if (listOptions == null) { | ||
| listOptions = new StripeSourceListOptions(); | ||
| } |
There was a problem hiding this comment.
Why do you need to do this? Is it to ensure that object is passed as source even if nothing is sent? Asking because we don't do this in other List methods such as customer
There was a problem hiding this comment.
Yes, exactly. The StripeSourceListOptions object will ensure that object=source is added as a query parameter in the request. This seemed less hacky than hardcoding the parameter in the URL, but it does force us to instantiate an object if none is provided.
| @@ -40,6 +40,19 @@ public virtual StripeSource Detach(string customerId, string sourceId, StripeReq | |||
| ); | |||
| } | |||
|
|
|||
| public virtual StripeList<StripeSource> List(string customerId, StripeSourceListOptions listOptions = null, StripeRequestOptions requestOptions = null) | |||
| { | |||
| var url = string.Format(Urls.CustomerSources, customerId); | |||
There was a problem hiding this comment.
Is it worth using a method to share the code? Not sure but that's what we do in other services (though the URL is used more than twice)
There was a problem hiding this comment.
The method is trivial enough that there's not much value in sharing it.
That said, other API methods on StripeSourceService should definitely use the Urls object like all other API methods rather than hardcoding the URLs. We should fix this separately.
| }; | ||
| var SourceBitcoin = sourceService.Create(SourceBitcoinCreateOptions); | ||
|
|
||
| SourceAttachOptions.Source = SourceBitcoin.Id; |
There was a problem hiding this comment.
Minor but I like having separate options instances. Jayme used to compare the type on the object to the info on the options object itself but here since you override you can't do this.
It's definite more verbose my way but I found it cleaner when I started to work on the library. Feel free to ignore
There was a problem hiding this comment.
Oops, not sure why I missed this comment earlier.
Yeah, if you need the options object in your test it's better to instantiate it separately. In this case the options object are only instantiated within the test constructor though, and only the resulting list objects are persisted and used in test assertions.
| SourceCard.Card.Brand.Should().Be("Visa"); | ||
|
|
||
| var SourceBitcoin = SourceListAll.Data[1]; | ||
| SourceBitcoin.Type.Should().Be(StripeSourceType.Bitcoin); |
There was a problem hiding this comment.
Shouldn't bitcoin be first? Aren't sources returned with most recent first? Or is it due to the default source always being first even if added before and this being undocumented?
There was a problem hiding this comment.
Yes, the card source is attached first and becomes the default source.
It's definitely not great practice to rely on that behavior in a test, and I should probably not have hardcoded the indices, but it's probably not worth putting in too much effort in those tests since we plan to 🔥 them and use stripe-mock Soon™️.
r? @remi-stripe
cc @stripe/api-libraries
Fixes #1062.
This PR adds support for listing sources on customers, with optional filtering by source type.
It should be noted that while source objects are top-level resources, they are not directly listable (they can only be listed when attached to a specific customer). I have named the method
Listso as to be consistent withStripeCardService/StripeBankAccountService, and I think the presence ofcustomerIdin the method's signature makes it clear that this method only applies to customers.